Skip to content

Add CustomDA proof validation interface #357

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 60 commits into
base: develop
Choose a base branch
from
Open

Add CustomDA proof validation interface #357

wants to merge 60 commits into from

Conversation

Tristan-Wilson
Copy link
Member

  • Introduce ICustomDAProofValidator interface for extensible DA proof validation
  • Implement ReferenceDAProofValidator with hash verification and chunk extraction
  • Integrate CustomDA validation into OneStepProverHostIo for preimage type 3
  • Add comprehensive test coverage for reference validator implementation

@Tristan-Wilson
Add CustomDA proof validation interface and reference implementation
ae40939 
- Introduce ICustomDAProofValidator interface for extensible DA proof validation
- Implement ReferenceDAProofValidator with hash verification and chunk extraction
- Integrate CustomDA validation into OneStepProverHostIo for preimage type 3
- Add comprehensive test coverage for reference validator implementation
@cla-bot cla-bot bot added the s label Jun 18, 2025
@gzeoneth gzeoneth changed the base branch from main to develop June 18, 2025 18:25
gzeoneth
gzeoneth reviewed Jun 23, 2025
View reviewed changes
src/osp/OneStepProverHostIo.sol Outdated
Comment on lines 33 to 41
ICustomDAProofValidator public customDAValidator;

function setCustomDAValidator(
ICustomDAProofValidator _validator
) external {
// TODO: Add appropriate access control
customDAValidator = _validator;
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we don't keep any storage in the OSP contract, so instead I think we have 2 options

  1. store the var in the rollup and have the OSP read from the rollup
  2. make this immutable and set in the constructor, but we have to consider how it works with the rollup creator

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer to make this immutable because this allow more deterministic behavior, otherwise the rollup owner may change the DA contract and make a assertion invalid.

gzeoneth and others added 4 commits July 12, 2025 02:35
@gzeoneth
test: add more coverage
7d0ab0b
@Tristan-Wilson
Add ValidatePreimage inst for CustomDA cert val OSP
e18761e 
- Add new ValidatePreimage instruction that validates CustomDA
  certificates
- Update ICustomDAProofValidator interface to pass full proof to
  validateCertificate for extensibility
- Implement certificate validation in ReferenceDA, just checking a
  simple version byte as an example
- OSP verifies prover's validity claim matches DA provider's
  validation result
@Tristan-Wilson
feat: add trusted signer validation to ReferenceDAProofValidator
9204f87 
Implement ECDSA signature validation for CustomDA certificates with configurable trusted signers. This demonstrates how validators can verify that certificates are signed by authorized parties before accepting them as valid.

Key changes:
- Add trustedSigners mapping to store authorized signer addresses
- Implement signature recovery and validation in validateCertificate()
- Update certificate format to include ECDSA signature components (v, r, s)
- Add comprehensive tests for signature validation scenarios
- Document revert vs return behavior for different validation failures
@Tristan-Wilson
Fix hostio range
555e3a2
@Tristan-Wilson Tristan-Wilson marked this pull request as ready for review September 9, 2025 07:37
Base automatically changed from deterministic-factory-deployments to develop September 9, 2025 19:31
@gzeoneth gzeoneth self-requested a review September 9, 2025 19:56
yahgwai
yahgwai reviewed Sep 16, 2025
View reviewed changes
@gzeoneth
Copy link
Member

will work on the review comments

@yahgwai yahgwai self-requested a review September 30, 2025 15:54
yahgwai
yahgwai previously approved these changes Sep 30, 2025
View reviewed changes
Move ReferenceDAProofValidator.sol out of contracts
8a9a3d8 
Since this is a reference implementation only, it's moved into the
nitro repo under contracts-local.
@Tristan-Wilson
Copy link
Member Author

I've moved the reference implementation into nitro's contracts-local directory.

Fix: Read certificate size from correct offset in validateAndCheckCer…
1160b95 
…tificate

Bug was introduced in 44b2eb4 when refactoring from assembly.
The certSize was being read from proof[0:] instead of proof[proofOffset:],
causing PROOF_TOO_SHORT errors when the validator tried to use garbage
data as the certificate size.
@gzeoneth gzeoneth changed the title Add CustomDA proof validation interface and reference implementation Add CustomDA proof validation interface Oct 16, 2025
Tristan-Wilson added a commit to OffchainLabs/nitro that referenced this pull request Oct 17, 2025
@Tristan-Wilson
Full ReferenceDA implementation
bd54e9b 
This commit moves the ReferenceDAProofValidator contract and tests from
nitro-contracts to contracts-local, as this is a reference
implementation that doesn't need to be part of the core nitro-contracts
package. The solidity contract was already reviewed in
OffchainLabs/nitro-contracts#357

Since the Reference DA contract is now available, this commit
activates contract-based certificate validation by uncommenting the
ValidateWithContract calls in certificate.go, reference_reader.go, and
reference_validator.go. These were previously disabled with TODO
comments waiting for contract merge.

This commit also includes some changes required for nitro-testnode to
work in CustomDA mode with Reference DA. It Ensures contracts are
available in Docker builds by copying both contracts/ and
contracts-local/ directories. It also adds ReferenceDA signing key to
config dump exclusion list to prevent accidental exposure of private
keys.  This change was merged into the custom-da branch in:
#3803

Other changes required that were needed for the standalone daprovider to
work with nitro-testnode were:
   - New parent-chain-node-url and parent-chain-connection-attempts
     config
   - L1 client creation in daprovider startup for ReferenceDA mode
This change was merged into the custom-da branch in:
#3819
Tristan-Wilson added a commit to OffchainLabs/nitro that referenced this pull request Oct 17, 2025
@Tristan-Wilson
Full ReferenceDA implementation
7443a3c 
This commit moves the ReferenceDAProofValidator contract and tests from
nitro-contracts to contracts-local, as this is a reference
implementation that doesn't need to be part of the core nitro-contracts
package. The solidity contract was already reviewed in
OffchainLabs/nitro-contracts#357

Since the Reference DA contract is now available, this commit
activates contract-based certificate validation by uncommenting the
ValidateWithContract calls in certificate.go, reference_reader.go, and
reference_validator.go. These were previously disabled with TODO
comments waiting for contract merge.

This commit also includes some changes required for nitro-testnode to
work in CustomDA mode with Reference DA. It Ensures contracts are
available in Docker builds by copying both contracts/ and
contracts-local/ directories. It also adds ReferenceDA signing key to
config dump exclusion list to prevent accidental exposure of private
keys.  This change was merged into the custom-da branch in:
#3803

Other changes required that were needed for the standalone daprovider to
work with nitro-testnode were:
   - New parent-chain-node-url and parent-chain-connection-attempts
     config
   - L1 client creation in daprovider startup for ReferenceDA mode
This change was merged into the custom-da branch in:
#3819
@Tristan-Wilson Tristan-Wilson mentioned this pull request Oct 17, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@yahgwai yahgwai yahgwai left review comments

@gzeoneth gzeoneth Awaiting requested review from gzeoneth

At least 1 approving review is required to merge this pull request.

Assignees

@gzeoneth gzeoneth

Labels

s

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@Tristan-Wilson @gzeoneth @yahgwai @godzillaba